-
Notifications
You must be signed in to change notification settings - Fork 89
Add cmdlets to change cli Settings of ESG #439
Conversation
There is not yet testsuite but i will be follow soon |
d5195e8
to
d5a4a48
Compare
Hi, Now with testsuite (and also add a testsuite for Enable/DisableSSH) Can you review/check ? |
jenkins test this please |
Hi Alexis - sorry for the large delay responding - only getting a chance to actually catch up on PowerNSX backlog now. Ive taken a quick look at the merge - and a few things occur. Firstly - I think this should be a part of set-nsxedge rather than separate cmdlets. I recently did some policy work and as an example, check out how set-nsxsecuritypolicy works - it can either accept updated XML, OR explicitly update specific properties. best of both worlds. I think this is the model that all set-xxx cmdlets should use now. To that end, set-nsxedge already does the first (Accepts updated XML) and I think this an all related base (appliance, cli etc) edge config should be handled here too. So your code should be pretty much copy/paste to update set-nsxedge... and your tests should be easy to move too... |
Ill add a few other minor comments in review too... |
for load balancer, site‐to‐site VPN, and NAT services. | ||
|
||
The Set-NsxcliSettings cmdlet configure the cli Settings of the ESG | ||
it is mandatory to specified the password... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why? any other edge xml update doesnt require this? are you saying no cli settings can be updated unless credential is specified? I havent tested this yet, but it sounds wrong.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Try and you will see that... (may be crazy when see that...)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i tested updating the banner text with the clisettings api - and yes it rejects it without credentials, however, i also tried it by updating the full edge api - and it works fine. Another reason for including in set-nsxedge :D
tests/integration/04.Edge.Tests.ps1
Outdated
@@ -660,6 +660,67 @@ Describe "Edge" { | |||
} | |||
} | |||
|
|||
Context "SSH" { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great that you are adding tests, but why are you including ssh tests here? should be separate PR...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok
it is already a separate commit... (i see this issue when write my testsuite...)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
its because you aren't branching from master. you should use master to track upstream, branch from it for every isolated PR you intend to raise. For this case, dont worry - as long as the PR tests pass when we execute the full suite against it
tests/integration/04.Edge.Tests.ps1
Outdated
#When deploy pstester ESG, the SSH is enabled | ||
$edge.cliSettings.remoteAccess | should be "true" | ||
#Need Always to set a password to change cliSettings | ||
Get-NsxEdge $name | Get-NsxcliSettings | Set-NsxCliSettings -password Vmware1!Vmware1! -remoteAccess:$false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you havent tested get-nsxclisettings yet - you need prior test that tests this (and you should use it two lines above too!) - this will probably be a moot point if you move these functions to set-nsxedge though...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do you have a example ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
all the other tests that test retrieval first.
tests/integration/04.Edge.Tests.ps1
Outdated
it "Change (SSH) username" { | ||
$edge = Get-NsxEdge $name | ||
#By default it is admin | ||
$edge.cliSettings.userName | should be "admin" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if we dont rely on it, then no point in testing for it...
tests/integration/04.Edge.Tests.ps1
Outdated
it "Change Password Expiry" { | ||
$edge = Get-NsxEdge $name | ||
#By default it is 99999 | ||
$edge.cliSettings.passwordExpiry | should be "99999" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
likewise
tests/integration/04.Edge.Tests.ps1
Outdated
$edge = Get-NsxEdge $name | ||
#By default it is 99999 | ||
$edge.cliSettings.passwordExpiry | should be "99999" | ||
Get-NsxEdge $name | Get-NsxcliSettings | Set-NsxCliSettings -password Vmware1!Vmware1! -passwordExpiry 42 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
haha! :D
not sure it is a good idea to include on set-nsxedge it will be a big cmdlets with a lot of parameter.. or for rename to *-NsxEdgecliSettings ! |
Lots of parameters aren't a problem, as long as the code is clean and the invocation is not confusing for users. I don't see anything here that is an issue. Also - given the comment inline about the fact that you can use the full edge api to update clisettings, i think it makes more sense in set-nsxedge. |
Move all set-nsxclisettings to set-nsxedge and remove get-nsxclisettings (or always kept ?) |
if you think its needed, im not opposed to having get-nsx_edge_clisettings, but given you can just go (get-nsxedge fred).cliSettings, I'm not sure I see the value! :D |
97bc7a5
to
77b6cc1
Compare
Updated (test suite and some other change) it is better ? |
password is still mandatory - it needs to not be, I showed you how to avoid that requirement in the API, and it is still a separate cmdlet for set - it needs to be part of set-nsxedge as previously commented. |
i ask about test suite if it is good... |
bc346a6
to
70465fa
Compare
Hi @nmbradford, can you look ? it is add now on set-nsxedge (but kept set-clisettings...) |
70465fa
to
f50032e
Compare
@nmbradford any news ? |
looks a lot better (havent had a chance to review the tests yet, but the changes in Set-NsxEdge look good. What is still a problem is that you have (nearly) duplicate code in Set-CliSettings (which is still also called the wrong thing as it doesnt include the word edge in it.) and still has a mandatory password. Get-CliSettings is also called the wrong thing. You need to either |
i will look to avoid duplicate code I prefer to kept name Get/set-nsxCliSettings because i can be also working for DLR.. (and not also Edge) |
There is a CLI in NSX other than edge/dlr so this wont be ok. If you are adamant about keeping a cli specific cmdlet, please name it as requested. Create a second set of cmdlets that are DLR specific that also wraps the same cmd. As repeatedly stated, my preference is for there to be no cli specific cmdlets. This is yet another reason for that. |
Remove [Get|Set]-nsxCliSettings and only kept Set-NsxEdge with cliSettings |
Tests Failed |
for Get/Set-NsxCliSettings
the SSH username need to be only lowercase and fix missing example for -passwordExpiry
use Set-NsxEdge for modify cliSettings
fixed correct usage of $null
it is ok ? |
jenkins test this please |
Tests Failed |
|
||
if ( $PsBoundParameters.ContainsKey('passwordExpiry') ) { | ||
if ( invoke-xpathquery -node $_Edge -querymethod SelectSingleNode -Query "child::cliSettings/passwordExpiry" ) { | ||
$_Edge.cliSettings.passwordExpiry = $passwordExpiry |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This also needs to be cast as a string.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On what release it don't work ? (because work for me on Windows with PowerShell 6/Core
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
needs to work on Powershell 5.1 on Windows
jenkins test this please |
Tests Failed |
jenkins test this please |
Tests Failed |
CLI tests have passed now. Overall tests failed due to cleanup error, so this one is good to go now. |
Add 2 new cmdlets for get and set cliSettings of ESG (or DLR)
Fix #431